Skip to content

Conversation

@Vinnl
Copy link
Contributor

@Vinnl Vinnl commented Aug 28, 2019

Tim has said in a call that it's weird that this line is there in the first place and should be removed, so I guess this doesn't need another review from him.

Fixes #105.

@Vinnl Vinnl requested a review from megoth August 28, 2019 15:47
@TallTed
Copy link
Contributor

TallTed commented Aug 28, 2019

Comments on #105 should be addressed before merging.

@Vinnl Vinnl requested a review from timbl August 28, 2019 16:10
@Vinnl
Copy link
Contributor Author

Vinnl commented Aug 28, 2019

Thanks @TallTed, I've added Tim as a reviewer now.

Copy link
Contributor

@megoth megoth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ^_^

@megoth megoth merged commit 8128960 into master Aug 28, 2019
@megoth megoth deleted the 105-409-error branch August 28, 2019 17:43
@TallTed
Copy link
Contributor

TallTed commented Aug 28, 2019

Comments on #105 seem to make clear this is not "don't store a field's default value" but "don't store this field's default value" (or perhaps "don't store this app's user preference default value(s)"), which makes much more sense.

@Vinnl
Copy link
Contributor Author

Vinnl commented Aug 29, 2019

@TallTed It's even "don't delete a field's default value from their Pod" - because that wouldn't work, because it's not in their Pod. The functionality you described would have to be implemented by the form that's using this field.

(I only thought of that today; it was late for me yesterday. Sorry!)

@TallTed
Copy link
Contributor

TallTed commented Aug 29, 2019

@Vinnl - It's not too late (for you; I don't have the necessary permissions on these) to change the title/summary of this PR (#109), nor of the associated issue (#105), to make things clearer for future readers.

@Vinnl
Copy link
Contributor Author

Vinnl commented Aug 29, 2019

@TallTed The title is actually correct, I think: because the default value was stored in the rdflib Store (i.e. in-memory), it would then try to delete it from the Pod. So this PR stops it from adding it to the Store, which then results in it not trying to delete it from the Pod. (And the issue title identifies the bug that resulted from that behaviour.)

Does that make sense? It's still nice if future readers can see this clarification in the PR comments :)

@TallTed
Copy link
Contributor

TallTed commented Aug 29, 2019

I suggest changing the title because this is the first, and often only, thing people read when reviewing past issues/PRs. Reading over all the comments requires much more effort -- and is only likely to happen if the title suggests that it's about what they're researching.

Title is now "Don't store a field's default value in the store"

I suggested "don't store this field's default value in the store"
or "don't store this app's user preference default value(s) in the store"

You replied with "don't delete a field's default value from their Pod" which does seem most accurate thus far.

Maybe "apps shouldn't try to save/delete a default field value to/from the user's pod/store" would be even better?

@Vinnl
Copy link
Contributor Author

Vinnl commented Aug 29, 2019

@TallTed Oh, sorry - lost the context of your previous comment, which actually included a suggestion. Too many update notifications! I'll change it right away.

@Vinnl Vinnl changed the title Don't store a field's default value in the store Don't store this field's default value in the store Aug 29, 2019
@brownhoward brownhoward added this to the Data Browser Release 2 milestone Oct 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

409 Error when attempting to edit Profile Highlight Color

6 participants